Skip to content

Fix dependency installation in python.yml GitHub Action#656

Merged
tturocy merged 17 commits intomasterfrom
update-gh-actions-requirements
Dec 3, 2025
Merged

Fix dependency installation in python.yml GitHub Action#656
tturocy merged 17 commits intomasterfrom
update-gh-actions-requirements

Conversation

@edwardchalstrey1
Copy link
Copy Markdown
Member

@edwardchalstrey1 edwardchalstrey1 commented Dec 2, 2025

Changes

  • Now GH action installs from the requirements.txt files used by tests and docs directly
  • Also moves the logic that skips running the tutorial tests from the GH action to the test file itself (don't run for <3.12), to avoid any inconsistencies with GH actions PR vs Push vs running locally
  • Removes the version pinning for doc and test requirements - this was actually preventing the installation of packages from working across Python versions, as the pinned package versions for earlier python versions were not always available. It was too stringent to pin these and I don't think there's much to be gained from having done so - if the latest versions of any of those packages do end up including changes that break our docs infrastructure, this will at worst mean that a docs build could fail, but that would be picked up in any PR and would not affect the current build. Might be worth discussing pinning of dependencies of pygambit itself in future though
  • See comment below: The paths-ignore in the GitHub action yml was wrongly preventing some checks from running. I don't think it's a big problem to remove this completely, whilst PRs take a long time to run, we don't do docs PRs in high enough volume/speed for this to be too much of a problem
  • Since Windows requires a specific manual installation of OpenSpiel, I have added a Warning that links to this in the How to run PyGambit tutorials on your computer and skipped running OpenSpiel notebook when running tests on Windows GH action. Also made a few other edits to this page whilst I was there.

@edwardchalstrey1
Copy link
Copy Markdown
Member Author

edwardchalstrey1 commented Dec 2, 2025

Hmm it seems that the rules I added for when to run python checks are incorrectly preventing those checks from running, even when we do actually edit files outside the excluded folders in some cases... hence problems were missed when merging previous PRs:

on:
  push:
    paths-ignore:
      - 'doc/**'
      - '.github/ISSUE_TEMPLATE/**'
      - 'README.md'
  pull_request:
    paths-ignore:
      - 'doc/**'
      - '.github/ISSUE_TEMPLATE/**'
      - 'README.md'

For now I'm going to remove these paths-ignore checks completely and address the address the issue @tturocy noticed in #655 and ensure matplotlib is included in the requirements

Edit: on second thought, the problem was simply that the openspiel PR only edited files within docs: https://github.com/gambitproject/gambit/pull/573/files

@edwardchalstrey1 edwardchalstrey1 changed the title Update dependency installation in GitHub Actions workflow Fix dependency installation in python.yml GitHub Action Dec 2, 2025
@edwardchalstrey1
Copy link
Copy Markdown
Member Author

edwardchalstrey1 commented Dec 2, 2025

  • Ah, another issue now the checks are running properly I didn't notice before, OpenSpiel install is throwing an error on Windows!

@edwardchalstrey1
Copy link
Copy Markdown
Member Author

Hi @tturocy if you review and merge this to master, then merge master into your branch for #655 that should solve the problem

@edwardchalstrey1 edwardchalstrey1 moved this from In progress to In review in Ed Chalstrey Gambit priorities Dec 2, 2025
@tturocy tturocy merged commit 3489b98 into master Dec 3, 2025
28 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in Ed Chalstrey Gambit priorities Dec 3, 2025
@tturocy tturocy deleted the update-gh-actions-requirements branch December 3, 2025 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants